Honestly this now feels it become completely unmaintainable and hardly understandable, can we not simply use an abstract base-class to share code instead of all this inlined lamda and hype-generic method calls?

Test Results

 1 523 files   -   301   1 523 suites   - 301   1h 15m 34s ⏱️ - 16m 26s
 7 918 tests ±    0   7 688 ✅  -     2  229 💤 +  1  0 ❌ ±0  1 🔥 +1 
22 184 runs   - 1 657  21 630 ✅  - 1 463  553 💤  - 195  0 ❌ ±0  1 🔥 +1 

For more details on these errors, see this check.

Results for commit 90ed170. ± Comparison against base commit 2e51edb.

UiTestSuite org.eclipse.ui.tests.api.ApiTestSuite org.eclipse.ui.tests.api.WorkbenchPluginTest ‑ testGetImageRegistryFromAdditionalDisplay

♻️ This comment has been updated with latest results.

Honestly this now feels it become completely unmaintainable and hardly understandable, can we not simply use an abstract base-class to share code instead of all this inlined lamda and hype-generic method calls?

I'm not so sure if that becomes much simpler, but I can have a look.
Actually this was intended as up-front work to reduce the duplicated changes for #2593. But maybe it's simpler to complete that first and make this a follow-up. Then we also know better was is needed for that.

While looking at both classes again I got the impression that actually the FileImageDescriptor class could be fully replaced by URLImageDescriptor. There are probably some corner cases that still have to be handled (e.g. resource not found), but in general I didn't saw reason to keep the FileImageDescriptor.
Do you or anybody else know one?

While looking at both classes again I got the impression that actually the FileImageDescriptor class could be fully replaced by URLImageDescriptor.

I have now taken this path further and didn't found a reason speaking against it. The tests should now pass as well.

This contains now much less, i.e. just one additional lambda in the zoom-image computation and is, from my side, ready for submission.
Does anybody want to review this?
If there are no objection respectively to request for more time to review, I plan to submit this tomorrow.

Thank you for your review. Then I think this is ready for submission.
Nevertheless I have extracted the simplification part out into a separate PR

• Simplify and clean-up URLImageDescriptor #2938

With that only a few a few changes remain in URLImageDescriptor for this change.

Once we have at least one working build I plan to submit this.

@HannesWell Are you aware that this change introduced a test failure in FileImageDescriptorTest? That test fails in every PR since this one was merged and it's also reproducible locally that this change introduced the failure. It was also reported by the according checks for this for 😉 https://github.com/eclipse-platform/eclipse.platform.ui/runs/41211453350

Are you aware that this change introduced a test failure in FileImageDescriptorTest? That test fails in every PR since this one was merged and it's also reproducible locally that this change introduced the failure.

My bad, I oversaw that and confused it with the other intermittent test-failure. Sorry and thanks for the heads-up.
Just created a fix to handle the kind of illegal files passed in in that test:

• Handle paths illegal on Windows in ImageDescriptor.createFromFile() #2946

* [Merge FileImageDescriptor into URLImageDescriptor and clean it up #2899 (comment)](https://github.com/eclipse-platform/eclipse.platform.ui/pull/2899#issuecomment-2833461917)

The link is broken.

PS. RCPTT tests also broke due to changed ImageDescriptor representations. Users have to recapture Table/Tree verifications. Nothing can be done to avoid or fix this situation, because RCPTT effectively relies on ImageDescriptor.toString() which is an implementation detail.

The link is broken.

It was indeed the wrong link, just fixed it.

PS. RCPTT tests also broke due to changed ImageDescriptor representations. Users have to recapture Table/Tree verifications. Nothing can be done to avoid or fix this situation, because RCPTT effectively relies on ImageDescriptor.toString() which is an implementation detail.

That's indeed unfortunate. But as you said, the exact result of toString() is not specified and one should not rely on it to have a specific value. If you think it's worth it and you can propose a simple fix to somehow restore the the result, we could 'fix' that.

If you think it's worth it

It is not. An application can change icons in its UI at any release and that's effectively what happened. Testing tools comparing icons should not pretend, than nothing has changed, so test failures are expected.

I only mention the problem for context, so that bigger picture is more visible.

If you think it's worth it

It is not. An application can change icons in its UI at any release and that's effectively what happened. Testing tools comparing icons should not pretend, than nothing has changed, so test failures are expected.

I only mention the problem for context, so that bigger picture is more visible.

Clients will likely have to update their tests anyway, due to the migration from PNGs to SVGs. So I think having both changes in a single release is actually good thing.

For example, this is an error now reported by our test suite:

Line 79: verify-true: Assertion of cells[4].image.path failed: expected:<...full/obj16/warn_tsk.png> but was:<...full/obj16/warn_tsk.svg>.

By the way previously we have used the Adapter Pattern to gather additional information.

e.g. Adapters.of(imageDescriptor, URL.class) would be much bettern than rely on a artificial toString ..

By the way previously we have used the Adapter Pattern to gather additional information.

e.g. Adapters.of(imageDescriptor, URL.class) would be much bettern than rely on a artificial toString ..

Unless the adapter is implemented by JFace, it is no better than direct downcast. And downcast requires internal API access. toString() was used to avoid use of internal APIs.

I see that adapter was added in 2022, but RCPTT still supports Eclipse Platform 2019-03, which makes the approach non-universal.

I see that adapter was added in 2022, but RCPTT still supports Eclipse Platform 2019-03, which makes the approach non-universal.

This does not mean one could try it first and then fallback to the old approach...